-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add router and ensembler webhook #395
Conversation
fb34c79
to
c879cee
Compare
7760b00
to
7d27829
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR! I've left a couple of tiny comments here and there but the most important one is the one about calling the webhooks whenever a router is updated, because that involves 2 steps, the deployment of a new router version and subsequently, the undeployment of the previous router version.
Another (equally tricky) change you might've left out is to add webhook calls to the 'DeployRouterVersion' method. This is similarly (or even more) confusing because there are two possible scenarios here - 1) there's just a simple deployment of a specific router version (no undeployment involved) and 2) there's a deployment of the specified router version + the undeployment of an existing deployed router version. Scenario 1 happens when there are no existing router versions deployed but scenario 2 happens when another router version was already deployed when that API endpoint gets called. You might need to poke a little more to find out some way to determine if you need 1 additional webhook call for the undeployment event for this API endpoint.
b207e5f
to
bcdbec4
Compare
18f0c0f
to
1a856b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thank you so much for the changes @deadlycoconuts!
Thanks a lot for looking at this PR again! Thanks a lot @bayu-aditya also for most of the work! :D I'll merge this PR now! |
Damn I had to approve this PR myself (to allow the merge button to appear) even though I have pushed changes to it and other people have approved it. It's probably because I selected the 'request changes' option when reviewing this PR and I need to be the one approving if I select that option 😑 |
Context
This PR to adding webhooks for Router and Ensembler for this action and event type.
OnRouterCreated
)OnRouterUpdated
)OnRouterDeleted
)OnRouterDeployed
)OnRouterUndeployed
)OnEnsemblerCreated
)OnEnsemblerUpdated
)OnEnsemblerDeleted
)For router, Turing will send webhook event with this body payload
Then for router deployment, Turing will send webhook event with this body payload
And for ensembler, Turing will send webhook event with this body payload
Changes
BaseController
/projects/{project_id}/routers
/projects/{project_id}/routers/{router_id}
/projects/{project_id}/routers/{router_id}
/projects/{project_id}/routers/{router_id}/deploy
/projects/{project_id}/routers/{router_id}/undeploy
/projects/{project_id}/ensemblers
/projects/{project_id}/ensemblers/{ensembler_id}
/projects/{project_id}/ensemblers/{ensembler_id}